Skip to content

fix(cli): seed INITIAL_PASSWORD only when no password is set (#11494) - #11501

Closed
ntdatt812 wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
ntdatt812:fix/11494-setup-initial-password
Closed

ntdatt812 wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
ntdatt812:fix/11494-setup-initial-password

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

Closes #11494.

omniroute setup read INITIAL_PASSWORD unconditionally and re-hashed it over whatever was already stored. A default npm install -g omniroute has that variable in the environment carrying CHANGEME, so any later setup run — including one whose only purpose was --add-provider — silently replaced the operator's own password with the well-known default, and printed ✔ Admin password configured while doing it.

Why the CLI is the odd one out

The server already treats the variable as a seed rather than an override — src/lib/auth/managementPassword.ts:

const bootstrapPassword =
  storedPassword ||
  getInitialPasswordValue(options.initialPassword ?? process.env.INITIAL_PASSWORD);

The stored hash wins there, and when the value is the well-known default the server logs a loud [AUTH][SECURITY] warning. The CLI path added in #8439 had neither guard. This makes the two agree: one line in resolvePassword, gated on the password that is already stored.

--password still wins, and a first install with nothing stored still seeds from the environment — which is what #8439 added the read for.

One behaviour change worth calling out

Interactively, with INITIAL_PASSWORD set and a password already stored, the user now sees the existing Set an admin password now? [y/N] prompt instead of having the env var applied silently. Answering N leaves the stored password alone. Before this change that prompt was unreachable whenever the variable was set.

I did not touch the packaging half of the report (an active .env inside the published tarball). package.json's files list ships .env.example, not .env, so whatever produces the installed .env is a separate question from this one, and this fix stands on its own either way: INITIAL_PASSWORD should not overwrite a set password no matter how it got into the environment.

Tests

Two layers, because they fail for different reasons.

tests/unit/cli/setup-initial-password-11494.test.ts (new, 4 assertions) — the decision alone, no database and no TTY. resolvePassword is now exported for this, matching how mergeSetupOptions is already exported and tested in tests/unit/cli/setup-provider-api-key.test.ts. The injected prompt calls assert.fail, so a regression that starts prompting non-interactively fails loudly instead of hanging.

tests/unit/cli-setup-command.test.ts (1 added) — the issue's reproduction end to end against a real SQLite file: set a password, run setup again with no --password while INITIAL_PASSWORD=CHANGEME, then assert the stored bcrypt hash still matches the operator's password and does not match CHANGEME.

Mutation-checked. Reverting the guard to the old unconditional if (process.env.INITIAL_PASSWORD):

  • the unit file goes 3 pass / 1 fail — only INITIAL_PASSWORD does not replace a password that is already set;
  • the DB file goes 6 pass / 1 fail — only a later setup run does not re-seed INITIAL_PASSWORD over the stored password.

The other three unit assertions pass under that mutation by design: they pin behaviour this PR preserves (#8439 seeding, --password precedence, the non-interactive no-op), not the behaviour it changes.

Commands run

npx tsx --test --test-force-exit tests/unit/cli-setup-command.test.ts \
                                 tests/unit/cli/setup-provider-api-key.test.ts \
                                 tests/unit/cli/setup-initial-password-11494.test.ts
→ 14 pass / 0 fail   (before this PR: 10 pass / 0 fail — the 4 new ones did not exist)

Both pre-existing INITIAL_PASSWORD tests in cli-setup-command.test.ts still pass unchanged: one seeds on a fresh data dir, where nothing is stored and the variable still applies; the other passes --password, which short-circuits ahead of the new condition.

Full npm run test:coverage was not run locally — the changed lines are the four resolvePassword branches plus the getSettings hoist in setupPassword, all executed by the tests above.

Local note, not a repo problem: tests/unit/cli-setup-command.test.ts imports better-sqlite3 directly and the package was absent from my checkout, so I installed it with npm i --no-save to run that file. package.json and package-lock.json are untouched by this branch.

`setup` read INITIAL_PASSWORD unconditionally and re-hashed it over whatever
was already stored. A default npm install has INITIAL_PASSWORD=CHANGEME in the
environment, so any later `setup` run -- including one that only meant to add a
provider -- silently replaced the operator's own password with the well-known
default, while printing "Admin password configured".

The server already treats the env var as a seed rather than an override:
`storedPassword || getInitialPasswordValue(...)` in
src/lib/auth/managementPassword.ts. The CLI now applies the same rule.

An explicit --password still wins, and a first install with no stored password
still seeds from the env var, which is what diegosouzapw#8439 added it for.

Closes diegosouzapw#11494
@ntdatt812

Copy link
Copy Markdown
Contributor Author

Nine checks are red. All nine are the base, not this diff — and the five new tests pass in CI.

The failing set is identical on unrelated PRs cut from the same base. #11496 and #11499 share none of this diff and fail exactly these nine:

Build (advisory)            Unit Tests fast-path (1/4)
Docs Gates (fast-path)      Unit Tests fast-path (2/4)
Fast Quality Gates          Unit Tests fast-path (3/4)
No new ESLint warnings      Unit Tests fast-path (4/4)
Vitest (fast-path)

Unit Tests fast-path (1/4) — the five failures there are agent-card assertions:

✖ GET /.well-known/agent.json returns 6 skills
✖ Agent Card includes list-capabilities skill entry
✖ list-capabilities entry has required tags [discovery, capabilities]
✖ list-capabilities entry has at least one example question
✖ Agent Card includes all 5 original skills

Nothing there reads the CLI or the settings store, and neither of this PR's test files appears in those logs.

Build (advisory) is the release/v3.8.51 production build, still broken because src/shared/utils/wsPath.ts is missing the sanitizeLiveWsPort / resolveLiveWsUrl block that #11388 added and this branch's per-file assembly dropped. #11465 is the fix and is approved but not yet queued; every PR on this base inherits that failure until it lands.

What this PR's own tests did in CI — pulled from the shard logs rather than inferred:

✔ INITIAL_PASSWORD does not replace a password that is already set (#11494)
✔ INITIAL_PASSWORD still seeds the first password (#8439)
✔ an explicit --password still wins over a stored one
✔ no INITIAL_PASSWORD and nothing stored still writes nothing non-interactively
✔ a later setup run does not re-seed INITIAL_PASSWORD over the stored password

Every other INITIAL_PASSWORD test in the repo also passes on this branch, including the ones that pin the server-side semantics this change was made to match:

✔ ensurePersistentManagementPasswordHash migrates INITIAL_PASSWORD into a persisted bcrypt hash
✔ auth login route lazily migrates INITIAL_PASSWORD to a persisted hash before validating
✔ settings route password update requires the current INITIAL_PASSWORD after lazy hash migration
✔ settings route password update rejects the wrong current password after migrating INITIAL_PASSWORD
✔ isAuthRequired stays enabled when INITIAL_PASSWORD is present
✔ INITIAL_PASSWORD marks onboarding as complete on first read
✔ setup command reads the admin password from INITIAL_PASSWORD when --password is not set
✔ setup command prioritizes an explicit --password flag over INITIAL_PASSWORD

Happy to rebase once the base is green.

@ntdatt812

Copy link
Copy Markdown
Contributor Author

Superseded by #11522 — closing this rather than rebasing it.

I went to rebase onto the new release/v3.8.51 tip (6eb7bf0ba) and hit a conflict in the exact function this PR changes, which is how I found it. Checked before concluding:

// a8dbf7bbb — fix(cli): preserve existing admin password during setup (#11522)
async function resolvePassword(opts, prompt, nonInteractive, settings) {
  if (opts.password !== undefined) return opts.password;
  if (!settings.password && process.env.INITIAL_PASSWORD) return process.env.INITIAL_PASSWORD;

Same root cause, same rule — INITIAL_PASSWORD seeds the first password only. And their opts.password !== undefined is the better of the two: an explicit --password "" is now treated as "the operator asked for no password" rather than falling through to the environment, which my if (opts.password) would have got wrong.

Their tests cover it too, including the case I thought was mine to add — setup command does not replace an existing password from INITIAL_PASSWORD at tests/unit/cli-setup-command.test.ts:254, alongside the two pre-existing INITIAL_PASSWORD cases. So there is no distinct coverage left to carry over either.

Nothing here to salvage. Thanks to whoever picked it up — the fix is in the right place.

@ntdatt812 ntdatt812 closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] CLI: any omniroute setup run silently resets the dashboard admin password to CHANGEME

1 participant